Skip to content

Conversation

@cebtenzzre
Copy link
Contributor

Continuation of #37

Since c73b53f, minja hangs when parsing the template {#, e.g.:

#include <minja/minja.hpp>
int main() {
    try {
        minja::Parser::parse("{#", {});
    } catch (...) {
        return 1;
    }
}

This program should exit with status code 1, but instead it hangs.

I found a fix that works even with the nonconforming Microsoft STL, and added a test.

Copy link
Contributor

@ochafik ochafik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @cebtenzzre !

Trying to understand why tests are all red before this can go in. Some windows failures are clearly unrelated (hendrikmuhs/ccache-action#287), others, not sure yet.

}
} else if (std::regex_search(it, end, match, non_text_open_regex)) {
if (!match.position()) {
assert(match[0] == "{#");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rather avoid asserts for now, could you throw something here instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed. I think some kind of assert macro should be added at some point, though clearly out of scope for this PR.

auto left = parseStringConcat();
if (!left) throw std::runtime_error("Expected left side of 'logical compare' expression");

static std::regex compare_tok(R"(==|!=|<=?|>=?|in\b|is\b|not[\r\n\s]+in\b)");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍😅 🙈

@cebtenzzre cebtenzzre force-pushed the fix-opencomment-hang branch from e97bb24 to 96ebff4 Compare February 8, 2025 16:16
@cebtenzzre
Copy link
Contributor Author

Is there any reason this repo uses plain merges instead of squash merges for PRs? A linear commit history like llama.cpp has is simpler and keeps fixup commits in PRs out of the main branch.

@ochafik
Copy link
Contributor

ochafik commented Feb 8, 2025

Is there any reason this repo uses plain merges instead of squash merges for PRs? A linear commit history like llama.cpp has is simpler and keeps fixup commits in PRs out of the main branch.

Main reason has been most of my prs contained sometimes unrelated changes so far (just like this one has the welcome \s simplification ;-)), happy to change going forward.

@ochafik ochafik merged commit 2f27203 into google:main Feb 8, 2025
3 of 11 checks passed
@ochafik
Copy link
Contributor

ochafik commented Feb 8, 2025

Thanks again @cebtenzzre !

ochafik added a commit to ochafik/minja that referenced this pull request Nov 2, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants